Add support for declarative ESD configuration#7
Conversation
|
I can take this review! Will need to learn a bit about ESD before I can approve, but that doesn't seem like wasted time since it's used in all fromsoft games apparently |
|
@zer0x64 Are you still planning to review this? It's blocking all the other PRs for 4.1.0, so at some point I'll just need to land it. |
Sorry for lack of updates. I'll sit on this tomorrow! If I don't, don't wait on me, I'm spread thinner than I thought these days 🫠 |
zer0x64
left a comment
zer0x64
left a comment
There was a problem hiding this comment.
Nothing major, only a few questions and recommendations.
On another note, like I said I'm spread thinner than expected these days, so I won't self-assign an issue like this until I learn a bit more about the codebase and have a bit less projects in parallel.
| events, | ||
| ESDDocumentation.DeserializeFromFile( | ||
| $@"{game.Dir}\Base\ds3.esd.json", | ||
| new ESDDocumentation.DocOptions() { Game = "ds3" } |
There was a problem hiding this comment.
Looking at this function, there's a lot of non-generic references to DS3. Is this because the other games uses another form, or is it because they are not implemented yet? (using this flow at least)
There was a problem hiding this comment.
The latter—we only have Archipelago support for DS3. You can see a draft of Sekiro support in #15, but I'm waiting to put that out for review until I can verify it with an end-to-end prototype.
| /// </summary> | ||
| public class ExistingState | ||
| { | ||
| // TODO: Group and state numbers are auto-assigned by the compiler and thus subject to |
There was a problem hiding this comment.
Won't to it right now, but I could work on the research side a bit on this when I'll come back if nobody started yet. I assume this is a major blocker for version-independant mod
There was a problem hiding this comment.
That would be helpful. It's not clear right now how important being version-independent is in the short term—DS3 and Sekiro are unlikely to get any more patches that modify their event code, and we probably won't get to ER before Tarnished Edition lands. But if we ever expand this to new games (or use it for other types of mods) it'll matter a lot.
| { | ||
| if (!esd.StateGroups.TryGetValue(0x7FFFFFFF - Group, out var group)) | ||
| { | ||
| throw new Exception($"ESD {esd.Name} doesn't have a state group {Group}"); |
There was a problem hiding this comment.
Just pointing out that the esd parameter here is only used for error formatting. Might or might not want to remove it altogether, depending wether you value error clarity or code bloat/readability more(since from a high level view, this argument is not used)
There was a problem hiding this comment.
This is a bit of a weirdness in how C# kinda hides variable assignments in if statements, but it is actually used—if this if statement succeeds, esd.StateGroups is used to assign group, which is then used below to assign state, which is what we actually do the edits to.
| /// </returns> | ||
| private bool MatchArguments(Command command, ESDDocumentation doc) | ||
| { | ||
| if (Arguments.Count > command.Arguments.Count) return false; |
There was a problem hiding this comment.
Suggestion: != instead of >. Might be missing some context here, but from an isolated look, if arguments matches, they should have the same Count
There was a problem hiding this comment.
It's an intentional feature that listing fewer arguments matches longer calls, to avoid a bunch of , null, null, null, null]s all over.
| var editTypes = 0; | ||
| if (Replace.Count > 0) editTypes++; | ||
| if (Set != null) editTypes++; | ||
| if (Arguments.Count > 0) editTypes++; | ||
| if (AndAlso != null) editTypes++; | ||
| if (OrElse != null) editTypes++; | ||
| if (editTypes > 1) | ||
| { | ||
| throw new Exception("Each ExpressionModifier may only contain one edit"); | ||
| } |
There was a problem hiding this comment.
See related comment above, an if statement seems clearer
There was a problem hiding this comment.
This one would be tricky to replace with a single if clause. Since we're checking if any two edits are active, it would need O(n^2) clauses to represent the full set of checks—one for each pair.
| var editTypes = 0; | ||
| if (Replace.Count > 0) editTypes++; | ||
| if (Set != null) editTypes++; | ||
| if (Arguments.Count > 0) editTypes++; | ||
| if (AndAlso != null) editTypes++; | ||
| if (OrElse != null) editTypes++; | ||
| if (editTypes > 1) | ||
| { | ||
| throw new Exception("Each ExpressionModifier may only contain one edit"); | ||
| } |
2 participants
Based on APIs added in fswap/SoulsIds#1